[vdo] Capture more information about vdo volumes #3717
Conversation
|
In this case I moved the 'lvs' command to the VDO plugin because that will guarantee that the command is run when the conditions for the other VDO command captures are met. If I add it to the LVM plugin, I'll have to add extra logic to make sure VDO is in place, and I thought it was a more natural place to have it in the VDO than the LVM one. But I can change it if the logic is not sound. |
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
1 similar comment
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
jcastill
force-pushed
the
jcastillo-capture-more-vdo-data
branch
2 times, most recently
from
c99b9b9
to
b4b5448
Compare
|
Added the missing comma and another extra command. With this change now we should capture both non-LVM and LVM VDO devices |
jcastill
force-pushed
the
jcastillo-capture-more-vdo-data
branch
from
b4b5448
to
23c7536
Compare
sos/report/plugins/vdo.py
Outdated
| vdo_cols = 'vdo_deduplication,vdo_index_state' | ||
| lvs_opts = '-a -o +' |
There was a problem hiding this comment.
Why the two extra variables, if you use either of them just once?
There was a problem hiding this comment.
Two reasons:
- The LVS command was too long before I removed the headers option, so using these two variables I was within the limit, and
- The request has a very big list of columns and I'll need to split the output in at least two variables to make sure is readable.
I'm still playing with the columns I need to display, so the name vdo_cols will probably change.
There was a problem hiding this comment.
OK, could be.. Bit redundant for the code, more readable for users.
There was a problem hiding this comment.
This is still a draft PR so I'll see how can I improve it
sos/report/plugins/vdo.py
Outdated
| vdo_cols = 'vdo_deduplication,vdo_index_state' | ||
| lvs_opts = '-a -o +' | ||
| lvm_vdos = self.collect_cmd_output(f"lvs {lvs_opts}{vdo_cols}") | ||
| for vdo in lvm_vdos['output'].splitlines(): |
There was a problem hiding this comment.
What if the lvs command would raise an error - is it safe to parse its output like this? (we often guard 'output' under if lvm_vdos['status'] == 0: - but here I see this plugin lacks this check (cf. lines 28-29).
There was a problem hiding this comment.
I'll do this, sorry I missed the check.
jcastill
force-pushed
the
jcastillo-capture-more-vdo-data
branch
2 times, most recently
from
abe4b00
to
7a682a1
Compare
jcastill
force-pushed
the
jcastillo-capture-more-vdo-data
branch
from
7a682a1
to
dfc31c6
Compare
|
After having a chat with VDO engineering, we've reworked the column output to have three captures that group the outputs that make more sense, instead of one or two with too many columns that makes them unreadable. |
Capture more information about vdo volumes Related: VDO-5797 Signed-off-by: Jose Castillo <[email protected]>
jcastill
force-pushed
the
jcastillo-capture-more-vdo-data
branch
from
dfc31c6
to
f2bb5e2
Compare
Capture more information about vdo volumes
Related: VDO-5797
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines